Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IUserPreferences #47658

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

IUserPreferences #47658

wants to merge 4 commits into from
+4,601 −198

Conversation

ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Aug 31, 2024 •
edited
Loading

  • replace IConfig's preferences-related method
  • tests

@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch 2 times, most recently from 6d81b97 to a9e2a38 Compare August 31, 2024 22:30
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 31, 2024
View reviewed changes
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
lib/public/UserPreferences/IUserPreferences.php Fixed Show fixed Hide fixed
lib/public/UserPreferences/IUserPreferences.php Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 1, 2024
View reviewed changes
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 1, 2024
View reviewed changes
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 1, 2024
View reviewed changes
lib/private/AllConfig.php Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 2, 2024
View reviewed changes
lib/private/AllConfig.php Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 2, 2024
View reviewed changes
lib/private/AllConfig.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch 2 times, most recently from 8edb1c1 to e510535 Compare September 2, 2024 02:31
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 2, 2024
View reviewed changes
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 2, 2024
View reviewed changes
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/AllConfig.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch 2 times, most recently from 32c209d to b80315b Compare September 2, 2024 14:57
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 2, 2024
View reviewed changes
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/AllConfig.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl marked this pull request as ready for review September 30, 2024 16:26
@ArtificialOwl ArtificialOwl added the 3. to review Waiting for reviews label Sep 30, 2024
@ArtificialOwl ArtificialOwl added this to the Nextcloud 31 milestone Sep 30, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 30, 2024
View reviewed changes
lib/private/UserPreferences.php Outdated
$value = $cache[$app][$key];
try {
$this->decryptSensitiveValue($userId, $app, $key, $value);
$value = $this->convertTypedValue($value, $typedAs ?? $this->getValueType($userId, (string)$app, $key, $lazy));

Check failure

Code scanning / Psalm

RedundantCast

Redundant cast to string
lib/private/UserPreferences.php Outdated
* @param string $value preference value
* @param bool $caseInsensitive non-case-sensitive search, only works if $value is a string
*
* @return list<string>

Check failure

Code scanning / Psalm

MoreSpecificReturnType

The declared return type 'list<string>' for OC\UserPreferences::searchUsersByValueString is more specific than the inferred return type 'array<array-key, mixed>'
lib/private/UserPreferences.php Outdated
* @since 31.0.0
*/
public function searchUsersByValueString(string $app, string $key, string $value, bool $caseInsensitive = false): array {
return $this->searchUsersByTypedValue($app, $key, $value, $caseInsensitive);

Check failure

Code scanning / Psalm

LessSpecificReturnStatement

The type 'array<array-key, mixed>' is more general than the declared return type 'list<string>' for OC\UserPreferences::searchUsersByValueString
lib/private/UserPreferences.php Outdated
* @param string $key preference key
* @param array $values list of preference values
*
* @return list<string>

Check failure

Code scanning / Psalm

MoreSpecificReturnType

The declared return type 'list<string>' for OC\UserPreferences::searchUsersByValues is more specific than the inferred return type 'array<array-key, mixed>'
lib/private/UserPreferences.php Outdated
* @since 31.0.0
*/
public function searchUsersByValues(string $app, string $key, array $values): array {
return $this->searchUsersByTypedValue($app, $key, $values);

Check failure

Code scanning / Psalm

LessSpecificReturnStatement

The type 'array<array-key, mixed>' is more general than the declared return type 'list<string>' for OC\UserPreferences::searchUsersByValues
come-nc
come-nc reviewed Oct 3, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stopped at line 1050 of UserPreferences, to be continued…

I dislike the sensitive flag thing, it should be a bool just like lazy so that all types var can be typed to the enum.

lib/private/AllConfig.php Outdated Show resolved Hide resolved
lib/private/AllConfig.php Outdated Show resolved Hide resolved
lib/private/AllConfig.php Outdated Show resolved Hide resolved
lib/private/UserPreferences.php Outdated
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OC;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid cluttering OC namespace please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move to \OC\UserPreferences\Manager ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving everything into \OC\Config and \OCP\Config

lib/private/UserPreferences.php Outdated Show resolved Hide resolved
lib/private/UserPreferences.php Outdated Show resolved Hide resolved
lib/private/UserPreferences.php Outdated Show resolved Hide resolved
lib/private/UserPreferences.php Outdated Show resolved Hide resolved
lib/private/UserPreferences.php Outdated Show resolved Hide resolved
lib/private/UserPreferences.php Outdated Show resolved Hide resolved
artonge
artonge reviewed Oct 3, 2024
View reviewed changes
lib/public/UserPreferences/IUserPreferences.php Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's keep it private for one major version to have the flexibility to change the API down the road?

susnux
susnux reviewed Oct 11, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note this somewhere (feel free to ignore):
For me the name if a bit confusing (I know the DB table is called like that),
as we have IConfig and IAppConfig I would expect IUserConfig as the name.

@ArtificialOwl
Copy link
Member Author

Just to note this somewhere (feel free to ignore): For me the name if a bit confusing (I know the DB table is called like that), as we have IConfig and IAppConfig I would expect IUserConfig as the name.

While I think grouping API might happens if we start today to create this API within \[OC|OCP]\Config/` but I am still fine with IUserPreferences

ArtificialOwl
ArtificialOwl commented Oct 17, 2024
View reviewed changes
lib/public/UserPreferences/ValueType.php Outdated Show resolved Hide resolved
lib/private/AllConfig.php Outdated Show resolved Hide resolved
core/Migrations/Version31000Date20240814184402.php Show resolved Hide resolved
lib/private/UserPreferences.php Outdated Show resolved Hide resolved
lib/private/UserPreferences.php Outdated
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OC;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving everything into \OC\Config and \OCP\Config

@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch 5 times, most recently from 6a3a4a1 to ebeff96 Compare October 21, 2024 19:31
@ArtificialOwl ArtificialOwl mentioned this pull request Oct 25, 2024
Merged
@skjnldsv skjnldsv added enhancement 2. developing Work in progress developer experience and removed 3. to review Waiting for reviews labels Nov 6, 2024
susnux
susnux reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is fine, but still want to confirm we want this name.

Reason: We use app config and system config and this was config->getUserValue.
So UserConfig might make more sense.

Yet all other places we have words for this refer it as user settings (documentation, occ commands etc), so maybe UserSettings. The only place we have that call it preferences is the DB table.

I am fine with anything regarding this if its agreed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc left review comments

@susnux susnux susnux left review comments

@artonge artonge artonge approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
2. developing Work in progress developer experience enhancement
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
5 participants
@ArtificialOwl @susnux @artonge @come-nc @skjnldsv